Skip to content

Update Wins overlay to use different SVG #8378

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
Oct 31, 2025
Merged

Update Wins overlay to use different SVG #8378

merged 1 commit into from
Oct 31, 2025

Conversation

@myronchen-git
Copy link
Member

@myronchen-git myronchen-git commented Oct 22, 2025 •
edited
Loading

Fixes #7937

What changes did you make?

  • Added GitHub icon and empty LinkedIn link to the wins overlay HTML.
  • Updated the updateOverlay function in wins.js to attach href and aria-label attributes to and hide/show the GitHub icons, as well as have the code for the LinkedIn icon utilize the new anchor element in the HTML.
  • Explicitly set the GitHub and LinkedIn icon sizes in the wins page SCSS.

Why did you make the changes (we will use this info to test)?

  • Since the icons are now being used from _includes\svg\, the JavaScript code in assets/js/wins.js could not access the _includes folder. The workaround is to use the include tag from Jekyll in the HTML.
  • The aria-label needed to be added to the GitHub icon to make it WCAG compliant.
  • The icon sizes needed to be set in the CSS, as the former sizes no longer applied.

CodeQL Alerts

After the PR has been submitted and the resulting GitHub actions/checks have been completed, developers should check the PR for CodeQL alert annotations.

Check the PR's comments. If present on your PR, the CodeQL alert looks similar as shown

Screenshot 2024-10-28 154514

Please let us know that you have checked for CodeQL alerts. Please do not dismiss alerts.

  • I have checked this PR for CodeQL alerts and none were found.
  • I found CodeQL alert(s), and (select one):
    • I have resolved the CodeQL alert(s) as noted
    • I believe the CodeQL alert(s) is a false positive (Merge Team will evaluate)
    • I have followed the Instructions below, but I am still stuck (Merge Team will evaluate)
Instructions for resolving CodeQL alerts

If CodeQL alert/annotations appear, refer to How to Resolve CodeQL alerts.

In general, CodeQL alerts should be resolved prior to PR reviews and merging

Screenshots of Proposed Changes To The Website (if any, please do not include screenshots of code changes)

  • No visual changes to the website, but verify this.

@myronchen-git myronchen-git added this to the x. Technical debt milestone Oct 22, 2025
@myronchen-git myronchen-git added the role: front end Tasks for front end developers label Oct 22, 2025
@myronchen-git myronchen-git added Complexity: Medium P-Feature: Wins Page https://www.hackforla.org/wins/ size: 1pt Can be done in 4-6 hours labels Oct 22, 2025
@HackforLABot HackforLABot mentioned this pull request Oct 22, 2025
Open
@github-actions
Copy link

github-actions bot commented Oct 22, 2025

Want to review this pull request? Take a look at this documentation for a step by step guide!

From your project repository, check out a new branch and test the changes.

git checkout -b myronchen-git-use-different-github-svg-for-wins-overlay-7937 gh-pages
git pull https://github.com/myronchen-git/website.git use-different-github-svg-for-wins-overlay-7937

@kdaca19xx kdaca19xx self-requested a review October 26, 2025 17:14
@kdaca19xx
Copy link
Member

kdaca19xx commented Oct 26, 2025

Availability: after 1pm (pacific)
ETA: 10/28

@HackforLABot HackforLABot mentioned this pull request Oct 26, 2025
Open
daras-cu
daras-cu requested changes Oct 28, 2025
View reviewed changes
Copy link
Member

@daras-cu daras-cu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @myronchen-git, great job on this issue, your code changes look good and the Wins page worked correctly when I tested it. The GitHub svg is displaying as it should at the correct size and you added the required aria-label attribute for accessibility.

One small fix before we merge-it looks like you may have an auto-formatter enabled that changed some of the white space in _wins-page.scss and wins.js. Can you please revert the lines that aren't part of your code changes? That way reviewers will have an easier time seeing the relevant changes, and the edit history on those files will be more clear.

Thanks for your work on this issue.

@github-project-automation github-project-automation bot moved this from PR Needs review to PRs being reviewed in P: HfLA Website: Project Board Oct 28, 2025
@myronchen-git myronchen-git force-pushed the use-different-github-svg-for-wins-overlay-7937 branch from 3277e48 to b2756b9 Compare October 28, 2025 19:20
@myronchen-git myronchen-git force-pushed the use-different-github-svg-for-wins-overlay-7937 branch from b2756b9 to cc1c87f Compare October 28, 2025 19:23
@myronchen-git
Copy link
Member Author

myronchen-git commented Oct 28, 2025

I have now removed the whitespace changes. Although, I do believe the files' whitespaces should be fixed.

daras-cu
daras-cu approved these changes Oct 29, 2025
View reviewed changes
Copy link
Member

@daras-cu daras-cu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @myronchen-git, all looks good now. I do agree, I think there are white space issues throughout the codebase, we may do an audit at some point after we work through some of the more pressing issues.

kdaca19xx
kdaca19xx approved these changes Oct 29, 2025
View reviewed changes
Copy link
Member

@kdaca19xx kdaca19xx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you, myronchen-git!

Things Done Well

  • The pull request was done with the correct branch.

  • There's a linked issue. I read/understood it.

  • Your edits in the Files Changed tab looks good.

Suggestions

none.

@github-project-automation github-project-automation bot moved this from PRs being reviewed to PRs ✅ waiting for merge team in P: HfLA Website: Project Board Oct 29, 2025
@DDVVPP DDVVPP merged commit 9f81fd1 into hackforla:gh-pages Oct 31, 2025
5 checks passed
@HackforLABot HackforLABot mentioned this pull request Oct 31, 2025
Open
@myronchen-git myronchen-git deleted the use-different-github-svg-for-wins-overlay-7937 branch November 7, 2025 23:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@daras-cu daras-cu daras-cu approved these changes

@kdaca19xx kdaca19xx kdaca19xx approved these changes

Assignees

No one assigned

Labels

Complexity: Medium P-Feature: Wins Page https://www.hackforla.org/wins/ role: front end Tasks for front end developers size: 1pt Can be done in 4-6 hours

Projects

P: HfLA Website: Project Board
Archived in project

Milestone

x. Technical debt

Development

Successfully merging this pull request may close these issues.

Update wins.js so the Wins overlay uses icon-github.svg

4 participants

@myronchen-git @kdaca19xx @daras-cu @DDVVPP